Skip to content

very simple and hacky path item parameter override test #145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 20, 2019
Merged

very simple and hacky path item parameter override test #145

merged 5 commits into from
Jun 20, 2019

Conversation

gweis
Copy link
Contributor

@gweis gweis commented Jun 18, 2019

Hi. This is a follow up on #140 and #144.

The fix in #144 is good enough for me and solves my use case. (Thanks again @p1c2u)

However I thought I play around a bit and test overriding parameters. Here are two very simple (and hacky) test cases to demonstrate that.

The issue of overriding Path Item Parameters with Operation Parameters is a bit more complex though. First it is possible to create an invalid Spec object by overriding a parameter with same name but different in attribute. The second part is parameter validation, which should only validate with the overridden parameter definition from the Operation object.

I believe it is a rather rare use case to override parameters, but I thought I'll put it up here. It does not hinder me using this fantastic library and I know that this pull-request is not really mergeable (I only put it together in one place to better demonstrate the two sides of this issue).

@codecov
Copy link

codecov bot commented Jun 18, 2019

Codecov Report

Merging #145 into master will decrease coverage by 0.55%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
- Coverage   96.75%   96.19%   -0.56%     
==========================================
  Files          58       58              
  Lines        1601     1602       +1     
==========================================
- Hits         1549     1541       -8     
- Misses         52       61       +9
Impacted Files Coverage Δ
openapi_core/validation/request/validators.py 100% <100%> (ø) ⬆️
openapi_core/compat.py 55.55% <0%> (-44.45%) ⬇️
openapi_core/validation/request/models.py 66.66% <0%> (-20.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ac4baf...3d23f17. Read the comment docs.

@p1c2u
Copy link
Collaborator

p1c2u commented Jun 19, 2019

@gweis thanks for the PR.

The issue of overriding Path Item Parameters with Operation Parameters is a bit more complex though. First it is possible to create an invalid Spec object by overriding a parameter with same name but different in attribute. The second part is parameter validation, which should only validate with the overridden parameter definition from the Operation object.

It's absolutely okay to have many parameters with the same name and different location. See https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#fixed-fields-7
It says:

A unique parameter is defined by a combination of a name and location.

@gweis
Copy link
Contributor Author

gweis commented Jun 19, 2019

A unique parameter is defined by a combination of a name and location.

@p1c2u thanks for pointing that out. (It's sometimes a bit hard to keep following all the little details)
This certainly simplifies that issue.

There is only one part left then, the part about validating overridden parameters. (I have updated the test cases, it's less tacky now :)

If you have any pointers on where in the code base to fix that, I can have a look and create a PR.

I assume somewhere in this class? https://github.com/p1c2u/openapi-core/blob/master/openapi_core/validation/request/validators.py#L12

@gweis
Copy link
Contributor Author

gweis commented Jun 19, 2019

I think I was able to put a fix together as well.

Copy link
Collaborator

@p1c2u p1c2u left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. LGTM 👍

@p1c2u p1c2u marked this pull request as ready for review June 20, 2019 15:29
@p1c2u p1c2u merged commit 0e30b71 into python-openapi:master Jun 20, 2019
@gweis gweis deleted the path-parameter-override branch June 20, 2019 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants